feat(hogql): placeholder expressions (part 1) #25091
Conversation
|
It looks like the code of |
mariusandra
force-pushed
the
placeholders-with-expr
branch
from
9946448
to
553a7f5
Compare
mariusandra
force-pushed
the
placeholders-with-expr
branch
from
553a7f5
to
08118e3
Compare
mariusandra
had a problem deploying
to
pypi-hogql-parser
GitHub Actions
Failure
mariusandra
had a problem deploying
to
pypi-hogql-parser
GitHub Actions
Failure
mariusandra
had a problem deploying
to
pypi-hogql-parser
GitHub Actions
Failure
mariusandra
temporarily deployed
to
pypi-hogql-parser
GitHub Actions
Inactive
mariusandra
temporarily deployed
to
pypi-hogql-parser
GitHub Actions
Inactive
…nto placeholders-with-expr
mariusandra
force-pushed
the
placeholders-with-expr
branch
from
f0317e4
to
bb3f072
Compare
mariusandra
temporarily deployed
to
pypi-hogql-parser
GitHub Actions
Inactive
| type: Optional[Type] = None | ||
|
|
||
| @cached_property | ||
| def placeholder_chain(self) -> str | None: |
There was a problem hiding this comment.
I'd return a list[str | int] like a normal ast.Field(...).chain - feels like this would be more aligned
| def visit_block(self, node: ast.Block): | ||
| # TODO: remove all this when we can use bytecode in placeholders | ||
| if len(node.declarations) > 1: | ||
| raise QueryError("Placeholders can only contain a single declaration") |
There was a problem hiding this comment.
Nit: "Blocks can only contain a single declaration"
There was a problem hiding this comment.
I'm not a fan of the language mix of "block" and "placeholder" - especially with removing ast.Placeholder node. I'd be in favour of removing all placeholder references in favour of "block" here
| @@ -40,6 +41,21 @@ class VariableDeclaration(Declaration): | |||
| expr: Optional[Expr] = None | |||
|
|
|||
|
|
|||
| @dataclass(kw_only=True) | |||
| class Block(Expr): | |||
There was a problem hiding this comment.
Having a util that creates a Block from similar interface as a ast.Placeholder could be nice:
def block_from_field_chain(chain: list[str | int]) -> ast.Block:
return ast.Block(....)
|
The biggest problem here is that it makes the parser a lot slower. Not substantially in my testing, but enough to cause a difference in CI --> one test that took 22min now times out at 30 😬. So work to do and alternatives to consider |
Problem
Pulls out the first chunk from #25040
Changes
ast.Placeholderwithast.Block.ast.Placeholder(chain=["hogql_val_1"])ast.Block(declarations=[ast.ExprStatement(expr=ast.Field(chain=["hogql_val_1"]))])How did you test this code?
Updated a lot of tests. MyPy was my friend.